-
Notifications
You must be signed in to change notification settings - Fork 173
Upgrade AG Grid to v34 and rewrite leaderboard routing #3319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughUpgrades AG Grid dependencies and introduces bootstrap modules for AG Grid and Sentry initialization. Migrates AG Grid theming from CSS classes to theme props across multiple components. Refactors leaderboard routing to nested routes with React Router loaders, removes old wrapper/route components, and scopes leaderboard styles. Minor router redirect status change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as index.tsx
participant AG as initializeAgGridModules
participant S as initializeSentryLogging
participant Store as Redux Store
participant Svc as Sentry SDK
User->>App: Load application
App->>AG: initializeAgGridModules()
AG->>AG: Build module list (env-dependent)
AG->>AG: ModuleRegistry.registerModules(...)
App->>S: initializeSentryLogging()
S->>Store: getState().session.userId
S->>Svc: Sentry.init({ dsn, env, release, integrations })
S->>Svc: Sentry.setUser({ id } | null)
sequenceDiagram
autonumber
participant RR as React Router
participant Store as Redux Store
participant L1 as leaderboardLoader
participant L2 as contestLeaderboardLoader
Note over RR: Load /courses/:courseId/leaderboard[..]
RR->>L1: Call loader with request
L1->>Store: Read session flags
alt both disabled
L1-->>RR: redirect(/courses/:id/not_found)
else at root path
alt overall enabled
L1-->>RR: redirect(.../leaderboard/overall) (replace)
else
L1-->>RR: redirect(.../leaderboard/contests) (replace)
end
else passthrough
L1-->>RR: null
end
Note over RR: Load /leaderboard/contests/[:contestId]/:type
RR->>L2: Call loader with params
L2->>Store: Read session flags and contests
alt contest feature disabled
L2-->>RR: redirect(not_found)
else no contestId
L2-->>RR: redirect(.../contests/{publishedId}/score) (replace)
else invalid type
L2-->>RR: redirect(not_found)
else
L2-->>RR: null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/academy/grading/subcomponents/GradingSubmissionsTable.tsx (1)
208-212
: Bug: Removing one filter unintentionally removes others with same id or same valueThe predicate uses
&&
with!==
, which removes any filter that matches either the id or the value. You likely intended to remove only the exact (id,value) pair.- const newFilters = columnFilters.filter(filter => filter.id !== id && filter.value !== value); + const newFilters = columnFilters.filter( + filter => !(filter.id === id && filter.value === value) + );src/pages/leaderboard/subcomponents/OverallLeaderboard.tsx (1)
121-123
: Don’t mutate Redux state objects; derive avatars in the callback insteadMutating
paginatedLeaderboard.rows
in place breaks Redux immutability and can cause subtle bugs. Derive the avatar URLs when passing data to the grid.Apply these diffs:
- Build derived rows in the success callback:
- successCallback( - paginatedLeaderboard.rows, - Math.min(paginatedLeaderboard.userCount, visibleEntries) - ); + const rowsWithAvatars = paginatedLeaderboard.rows.map((row: LeaderboardRow) => ({ + ...row, + avatar: `/assets/Sample_Profile_${convertToRandomNumber(row.username)}.jpg` + })); + successCallback( + rowsWithAvatars, + Math.min(paginatedLeaderboard.userCount, visibleEntries) + );
- Remove the state mutation outside:
- paginatedLeaderboard.rows.map((row: LeaderboardRow) => { - row.avatar = `/assets/Sample_Profile_${convertToRandomNumber(row.username)}.jpg`; - });
- Optionally, include
visibleEntries
in the dependency array so row count updates propagate:- }, [paginatedLeaderboard]); + }, [paginatedLeaderboard, visibleEntries]);Also applies to: 113-118, 119-119
🧹 Nitpick comments (17)
src/pages/academy/grading/subcomponents/GradingSubmissionsTable.tsx (2)
3-3
: Use type-only imports for AG Grid typesImport
ColDef
andCellClickedEvent
as types to avoid bringing type-only symbols into the runtime bundle and to stay consistent with the rest of the PR.-import { CellClickedEvent, ColDef, themeQuartz } from 'ag-grid-community'; +import { type CellClickedEvent, type ColDef, themeQuartz } from 'ag-grid-community';
421-434
: Type safety for filter model parsing
cellFilters
is currently untyped andkey
is astring
. Tighten the types to matchColumnFields
and guard the filter value.- const cellFilters = []; - for (const [key, { filter: query }] of Object.entries(filters)) { + const cellFilters: { id: ColumnFields; value: string }[] = []; + for (const [key, { filter: query } as any] of Object.entries(filters)) { switch (key) { // Fields that BE supports filtering on case ColumnFields.studentName: case ColumnFields.studentUsername: case ColumnFields.groupName: - cellFilters.push({ id: key, value: query }); + cellFilters.push({ id: key as ColumnFields, value: String(query ?? '') }); break; } }src/commons/sourceRecorder/SourceRecorderTable.tsx (1)
9-9
: Nit: use type-only imports for AG Grid types for consistencyFor consistency with other files in this PR (and to avoid runtime imports of types), consider switching to type-only imports.
-import { ColDef, GridApi, GridReadyEvent } from 'ag-grid-community'; +import { type ColDef, type GridApi, type GridReadyEvent } from 'ag-grid-community';src/pages/leaderboard/subcomponents/ContestLeaderboard.tsx (1)
175-178
: Minor: Pagination condition on Top 3 grid is never true
top3
has at most 3 rows, sopagination={top3.length > 10}
is always false. Drop pagination props for clarity.- pagination={top3.length > 10} - paginationPageSize={10} - paginationPageSizeSelector={[10]} + pagination={false}src/pages/stories/StoriesTable.tsx (1)
3-3
: Nit: importColDef
as a type-only import for consistency and bundle hygieneMatches the pattern used elsewhere in this PR.
-import { ColDef, themeQuartz } from 'ag-grid-community'; +import { type ColDef, themeQuartz } from 'ag-grid-community';src/routes/routerConfig.ts (1)
98-106
: Confirm redirect status semanticsI didn’t find any other
redirect(…, { status: … })
calls or usages of a401
status in the repo. If you intentionally dropped the401
and are fine with the default302
for unauthenticated users, no further changes are needed. Otherwise, to preserve “unauthorized” semantics in SSR, analytics or error-boundary logic, restore the explicit401
:- return redirect('/login'); + return redirect('/login', { status: 401 });src/index.tsx (1)
14-20
: Centralized bootstrapping looks goodCalling initializeSentryLogging and initializeAgGridModules at startup is the right place to ensure both are configured before any routes or grids mount.
Minor: during HMR these can run more than once; ensure the initializers are idempotent (ModuleRegistry is tolerant, and Sentry.init is safe when DSN is unset, but guarding can reduce console noise).
src/bootstrap/agGrid.ts (2)
18-31
: Don’t include ValidationModule in production bundleValidationModule is meant for development diagnostics. It’s already conditionally added below for development; including it in productionModules bloats the production bundle and can double-register the module.
Remove it from productionModules:
const productionModules: readonly Module[] = [ CellStyleModule, ClientSideRowModelModule, ColumnApiModule, ColumnAutoSizeModule, CsvExportModule, DateFilterModule, InfiniteRowModelModule, PaginationModule, RowDragModule, TextEditorModule, - TextFilterModule, - ValidationModule + TextFilterModule ];
33-41
: Optional: guard against duplicate registration during HMRModuleRegistry.registerModules is idempotent, but in dev HMR can cause repeated calls and noisy logs. You can add a simple guard:
export const initializeAgGridModules = () => { - const modulesToLoad = [...productionModules]; + // Avoid duplicate registration during HMR/dev + if (typeof window !== 'undefined' && (window as any).__AG_GRID_MODULES_REGISTERED__) { + return; + } + const modulesToLoad = [...productionModules]; @@ ModuleRegistry.registerModules(modulesToLoad); + if (typeof window !== 'undefined') { + (window as any).__AG_GRID_MODULES_REGISTERED__ = true; + } };src/pages/academy/adminPanel/subcomponents/assessmentConfigPanel/AssessmentConfigPanel.tsx (2)
320-323
: Make row updates robust under reordering with getRowIdIndex-based updates can desync after drag reordering. Provide stable row IDs so the grid and your state reconcile reliably.
Apply within this component:
- <AgGridReact + <AgGridReact + getRowId={params => String(params.data.assessmentConfigId)} theme={themeBalham}This avoids relying on
rowIndex
when callingsetDataValue
or handling drag moves.
3-9
: Verified themeBalham in ag-grid-community v34.1.1 + optional centralization
themeBalham is exported in your installed version (^34.1.1) and in use alongside themeAlpine and themeQuartz across the app. No breaking imports detected. To keep themes consistent, you may optionally centralize theme selection:• Major usages:
– src/pages/stories/StoriesTable.tsx (themeQuartz)
– src/pages/leaderboard/subcomponents/OverallLeaderboard.tsx (themeAlpine)
– src/pages/academy/dashboard/Dashboard.tsx (themeBalham)
– …and ~10 more components under src/pages/academy/adminPanel and commonsSuggested refactor (optional):
• Create a single module (e.g. src/config/agGridTheme.ts) exporting your default theme constant, and import that everywhere instead of importing raw theme* values.src/pages/academy/academyRoutes.ts (1)
10-13
: Loader location/namingImporting loaders from
../leaderboard/subcomponents/leaderboardUtils
blurs responsibilities. Consider moving them to a non-UI module (e.g.,../leaderboard/loaders
or../leaderboard/routes
) to better separate routing/data from presentation.src/pages/leaderboard/subcomponents/leaderboardUtils.ts (2)
11-15
: Avoid dynamic RegExp from variable input; use exact path comparisonA regex built from variable input can be fragile and is flagged by static analysis. You don’t need regex here; exact path equality is sufficient and safer.
Apply this diff:
- const rootRegex = new RegExp(`^${baseUrl}/leaderboard/?$`); - if (!rootRegex.test(path)) { + const isRoot = path === `${baseUrl}/leaderboard` || path === `${baseUrl}/leaderboard/`; + if (!isRoot) { // Don't try to redirect subpaths, pass through and proceed return null; }
10-10
: Typo in commentMinor nit: “If if it” -> “If it is”.
Apply this diff:
- // If if it a sub-path, pass through transparently + // If it is a sub-path, pass through transparentlysrc/pages/leaderboard/subcomponents/OverallLeaderboard.tsx (3)
46-53
: Use width instead of flex on the avatar image
flex: '40px'
on an img is odd and likely unintended. Use a fixed width for the avatar.Apply this diff:
- style={{ flex: '40px', height: '40px', borderRadius: '50%' }} + style={{ width: 40, height: 40, borderRadius: '50%' }}
28-57
: Prefer typing cell renderer paramsThe
any
typing in cellRenderers loses safety and IntelliSense. AG Grid provides renderer param types.Example:
import type { ICellRendererParams, ValueGetterParams } from 'ag-grid-community'; // ... cellRenderer: (params: ICellRendererParams<LeaderboardRow, number>) => { /* ... */ }
17-26
: Seed function slices the first char; confirm it’s intended for usernames
convertToRandomNumber
slices off the first character (id.slice(1)
). This made sense if IDs had a prefix (e.g.,S123
), but you’re now passingrow.username
. If usernames don’t have a known prefix, slicing could reduce entropy and cause collisions.If unnecessary, consider:
- const str = id.slice(1); + const str = id;Or document the assumption about the first character in a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (27)
package.json
(1 hunks)src/bootstrap/agGrid.ts
(1 hunks)src/bootstrap/sentry.ts
(1 hunks)src/commons/sourceRecorder/SourceRecorderTable.tsx
(7 hunks)src/index.tsx
(1 hunks)src/pages/academy/academyRoutes.ts
(2 hunks)src/pages/academy/adminPanel/AdminPanel.tsx
(0 hunks)src/pages/academy/adminPanel/subcomponents/AddStoriesUserPanel.tsx
(2 hunks)src/pages/academy/adminPanel/subcomponents/AddUserPanel.tsx
(2 hunks)src/pages/academy/adminPanel/subcomponents/assessmentConfigPanel/AssessmentConfigPanel.tsx
(2 hunks)src/pages/academy/adminPanel/subcomponents/storiesUserConfigPanel/StoriesUserConfigPanel.tsx
(2 hunks)src/pages/academy/adminPanel/subcomponents/userConfigPanel/UserConfigPanel.tsx
(2 hunks)src/pages/academy/dashboard/Dashboard.tsx
(2 hunks)src/pages/academy/grading/subcomponents/GradingSubmissionsTable.tsx
(3 hunks)src/pages/academy/groundControl/GroundControl.tsx
(2 hunks)src/pages/leaderboard/Leaderboard.tsx
(0 hunks)src/pages/leaderboard/subcomponents/ContestLeaderboard.tsx
(5 hunks)src/pages/leaderboard/subcomponents/ContestLeaderboardWrapper.tsx
(1 hunks)src/pages/leaderboard/subcomponents/LeaderboardDropdown.tsx
(2 hunks)src/pages/leaderboard/subcomponents/LeaderboardExportButton.tsx
(0 hunks)src/pages/leaderboard/subcomponents/LeaderboardPodium.tsx
(0 hunks)src/pages/leaderboard/subcomponents/OverallLeaderboard.tsx
(2 hunks)src/pages/leaderboard/subcomponents/OverallLeaderboardWrapper.tsx
(0 hunks)src/pages/leaderboard/subcomponents/leaderboardUtils.ts
(1 hunks)src/pages/stories/StoriesTable.tsx
(2 hunks)src/routes/routerConfig.ts
(1 hunks)src/styles/Leaderboard.scss
(1 hunks)
💤 Files with no reviewable changes (5)
- src/pages/academy/adminPanel/AdminPanel.tsx
- src/pages/leaderboard/subcomponents/OverallLeaderboardWrapper.tsx
- src/pages/leaderboard/Leaderboard.tsx
- src/pages/leaderboard/subcomponents/LeaderboardExportButton.tsx
- src/pages/leaderboard/subcomponents/LeaderboardPodium.tsx
🧰 Additional context used
🪛 ast-grep (0.38.6)
src/pages/leaderboard/subcomponents/leaderboardUtils.ts
[warning] 11-11: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${baseUrl}/leaderboard/?$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (19)
src/pages/academy/grading/subcomponents/GradingSubmissionsTable.tsx (2)
393-396
: AG Grid v34 theming viathemeQuartz
prop looks goodGood migration away from CSS theme classes to the
theme
prop on AgGridReact.
412-415
: Row selection API migration looks correctUsing the new
rowSelection
object withmode: 'singleRow'
andenableClickSelection
maps the oldsuppressRowClickSelection
behavior accurately.src/pages/academy/adminPanel/subcomponents/AddUserPanel.tsx (1)
15-15
: Solid migration to theme prop + type-onlyColDef
theme={themeBalham}
on AgGridReact is correct for v34.- Container no longer needs
ag-theme-*
classes.ColDef
imported as a type-only symbol keeps runtime clean.Also applies to: 53-53, 55-55
src/commons/sourceRecorder/SourceRecorderTable.tsx (2)
65-65
: Header menu suppression: correct property for v34Replacing
suppressMenu
withsuppressHeaderMenuButton
is the right API in v34 for hiding the header menu button.Also applies to: 76-76, 84-84, 94-94, 105-105, 118-118
10-10
: Balham theme via prop is correctly appliedImporting
themeBalham
and passing it viatheme={themeBalham}
is the correct v34 approach. Container can omitag-theme-*
classes.Also applies to: 171-171
src/pages/academy/adminPanel/subcomponents/AddStoriesUserPanel.tsx (1)
15-15
: Theming and typing changes look good
- Type-only
ColDef
import is correct.- The grid uses
theme={themeBalham}
and no longer relies on CSS theme classes.Also applies to: 53-53, 55-55
src/pages/academy/adminPanel/subcomponents/storiesUserConfigPanel/StoriesUserConfigPanel.tsx (1)
2-2
: Good alignment with v34 theming and typing
- Type-only imports for AG Grid types are correct.
theme={themeBalham}
migration looks good; wrapper class no longer needsag-theme-*
.Also applies to: 68-68, 70-70
src/pages/academy/adminPanel/subcomponents/userConfigPanel/UserConfigPanel.tsx (1)
2-2
: Type-only imports + theme object import look goodUsing type-only imports reduces bundle size;
themeBalham
import aligns with AG Grid v34 theming.src/pages/leaderboard/subcomponents/ContestLeaderboardWrapper.tsx (1)
17-18
: Lazy-route export alias pattern is correctExporting
Component
for React Router lazy routes is consistent and fine.src/pages/leaderboard/subcomponents/ContestLeaderboard.tsx (3)
3-3
: Theme import + type-only ColDef are aligned with AG Grid v34Using
themeAlpine
via the theme prop and a type-onlyColDef
import is the right approach.
22-29
: Stronger prop typing and destructuring LGTMNarrowing
type
to a union and passing the contest object directly simplifies the code and improves type safety.
167-171
: Adopting theme props for both grids looks goodPassing
theme={themeAlpine}
directly is consistent with the new API and removes reliance on theme CSS classes.Also applies to: 184-188
package.json (1)
51-52
: AG Grid v34 migration coverage completeAudit results:
- No occurrences of
ag-theme-*
insrc/**/*.scss
,.css
,.ts
, or.tsx
.ModuleRegistry.registerModules(modulesToLoad)
is present atsrc/bootstrap/agGrid.ts:40
.Migration is confirmed complete.
src/pages/stories/StoriesTable.tsx (1)
65-68
: Theme prop adoption looks goodUsing
theme={themeQuartz}
with a plain container div is correct for v34.src/pages/academy/dashboard/Dashboard.tsx (1)
29-41
: AG Grid v34 theme migration looks correctUsing theme={themeBalham} and dropping the CSS class-based theme is in line with v34. Grid props remain compatible, and removing the theme class from the container is fine.
Ensure that other grids follow the same approach for consistency.
src/pages/academy/groundControl/GroundControl.tsx (2)
11-11
: LGTM on type-only AG Grid imports and theme objectType-only imports reduce bundle size; the
themeBalham
runtime import aligns with the theme prop approach.
199-202
: LGTM on theme prop usageSwitching from
ag-theme-...
classes totheme={themeBalham}
onAgGridReact
is consistent with the v34 theming model.src/pages/leaderboard/subcomponents/leaderboardUtils.ts (1)
1-1
: ’replace’ helper is available in React Router v7.6.2
Verified that package.json specifies"react-router": "^7.6.2"
and thatreplace
is already imported in:
- src/routes/routerConfig.ts
- src/pages/leaderboard/subcomponents/leaderboardUtils.ts
- src/pages/academy/academyRoutes.ts
No further action needed.
src/pages/leaderboard/subcomponents/OverallLeaderboard.tsx (1)
3-5
: AG Grid v34 dependencies and API usage verified
- package.json lists both ag-grid-community and ag-grid-react at ^34.1.1
- themeAlpine is only used in:
• src/pages/leaderboard/subcomponents/ContestLeaderboard.tsx (lines 169–171, 186–188)
• src/pages/leaderboard/subcomponents/OverallLeaderboard.tsx (lines 140–142)- suppressCellFocus and suppressMovableColumns appear across tables but are supported in v34; no deprecations detected for these flags
All checks pass for AG Grid v34—no further action required.
src/pages/academy/adminPanel/subcomponents/userConfigPanel/UserConfigPanel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Description
Supersedes #3214. Also removed some hardcoding and rewrote leaderboard routing logic to be simpler.
Also various bug fixes.
Type of change
How to test
Checklist